Thanks @someth2say for this! Apologies for not getting to the review sooner. We have Red Hat summit going on this week so that will take up the majority of my time this week. I will get to this on Friday if that's ok?

Also you mentioned that this supercedes #237 - am I ok to close that one?

One thing I do notice off the bat is that in the generate-k8s-resources.sh script, you are assuming only ever a single "jvm" and "native". Even though right now we only have 1 JVM version (17) & 1 native version (17), we left it open on purpose to be able to support multiple jvm versions. For example, when Java 21 comes out, we may want to support both 17 & 21 for a period of time. The generate-k8s-resources.sh script needs to support that.

@edeandrea yes I think we're good to close that PR, I can close it now.

@someth2say thank you for taking this over! is there any chance I can find a helm chart that gets generated? I don't see any. I also remember @edeandrea mentioning that having separate scripts for the k8s resources and helm charts might be better from a maintenance standpoint, but I think the script looks good to me with both combined (and I think makes it so that the build doesn't need to be duplicated)

One thing I do notice off the bat is that in the generate-k8s-resources.sh script, you are assuming only ever a single "jvm" and "native". Even though right now we only have 1 JVM version (17) & 1 native version (17), we left it open on purpose to be able to support multiple jvm versions. For example, when Java 21 comes out, we may want to support both 17 & 21 for a period of time. The generate-k8s-resources.sh script needs to support that.

Mmmm... not sure that is true.
We declare two kinds, as you said: jvm and native. Then, for each kind, the script builds a version_tag, based on the javaVersions to be used in this kind.
So, currently we only have Java 17, so the version_tags are java17 for the kind jvm, and native for the kind native.
If we decide to add Java 21, then the version_tags would be java17 and java21 for the kind jvm, and native for the kind native.

I retained the conditional setting the javaVersions from main explicitly to support future JVM versions.

Maybe am I missing some point?

@edeandrea yes I think we're good to close that PR, I can close it now.

👍

is there any chance I can find a helm chart that gets generated? I don't see any.

The charts will be generated by the GH workflow Create deploy resources, and then commited back to the repo.
TBH I didn´t test it, but neither I changed it, so I guess it is working.

Nevertheless, I just realized that the workflow seems to have not been running for a while:

@edeandrea shouldn´t this workflow be executed automatically on every merge on main? Otherwise, we should run it manually.

I also remember @edeandrea mentioning that having separate scripts for the k8s resources and helm charts might be better from a maintenance standpoint, but I think the script looks good to me with both combined (and I think makes it so that the build doesn't need to be duplicated)

Yes, that was also my first approach.
But then I realized that would require doing the same Quakus build twice (one for k8s and one for helm). Given that the build is (by far) the step that takes more time and resources, it would be a waste to not reuse the build.
Also, both scripts would be 90% the same, so the maintenance win would not be that much (IMHO, at least).

Mmmm... not sure that is true.
We declare two kinds, as you said: jvm and native. Then, for each kind, the script builds a version_tag, based on the javaVersions to be used in this kind.
So, currently we only have Java 17, so the version_tags are java17 for the kind jvm, and native for the kind native.
If we decide to add Java 21, then the version_tags would be java17 and java21 for the kind jvm, and native for the kind native.

I retained the conditional setting the javaVersions from main explicitly to support future JVM versions.

Maybe am I missing some point?

Maybe I missed something as I only took a quick look :) I'll have a more thorough look on Friday.

Nevertheless, I just realized that the workflow seems to have not been running for a while:

@edeandrea shouldn´t this workflow be executed automatically on every merge on main? Otherwise, we should run it manually.

It should run on each commit to main. I'll need to figure out why it's not. I feel like it did run way more recently. Another thing for my Todo list on Friday.

You may need to alter the workflow to add any new deploy/helm resources to the git add that it does.

I also remember @edeandrea mentioning that having separate scripts for the k8s resources and helm charts might be better from a maintenance standpoint, but I think the script looks good to me with both combined (and I think makes it so that the build doesn't need to be duplicated)

Yes, that was also my first approach.
But then I realized that would require doing the same Quakus build twice (one for k8s and one for helm). Given that the build is (by far) the step that takes more time and resources, it would be a waste to not reuse the build.
Also, both scripts would be 90% the same, so the maintenance win would not be that much (IMHO, at least).

Yes @someth2say and I discussed this @ingmarfjolla after we realized we could reuse the same Quarkus build for the helm charts.

The create-deploy-resources workflow did run last week. That workflow doesn't run as a standalone workflow. It's included at the end of the build and push container images workflow.

It's done that way because it's templatized as to which branch to commit to. It is reused on all the different branches and accepts parameters from the calling workflow.

I also noticed that there is no chart for the ui-super-heroes nor the monitoring stuff. Were those forgotten?

For the monitoring stuff, you will probably have to hand-craft a chart inside the /monitoring folder (something like /monitoring/helm, similar to what we do for docker-compose (/monitoring/docker-compose) and k8s (/monitoring/k8s).

The generate-k8s-resources.sh script then creates the various monitoring descriptors within /deploy/docker-compose and /deploy/k8s. The helm charts will have to have the deployment type variants (knative, kubernetes, minikube, openshift) as well.

Similar for the ui-super-heroes. You'll probably have to hand-craft a chart following the same pattern(s) used for the k8s descriptors.

No, not forgotten, but still pending :)
I agree with your approach, I will manually generate the charts.

Also, I don't see that any of the documentation (each individual project README nor the main repo README) has been updated with any info about the helm charts nor how to use them. I would think in the main README as well as in each individual project in the Deploying to Kubernetes sections there should be a sub-section about using the Helm charts.

Yes, documentation still pending.
Thanks for the heads up!

I also ran the generate-k8s-resources.sh script and noticed a few things:

1. The note on the PR mentioned there was no "uber" chart yet, but after running I do see a /deploy/helm directory with stuff in there. What is that stuff that's in there?

That was an error in the script. I gave the uber-chart a try, but then I forgot to remove the code doing that.
It should be out now.

2. For each app in the deploy/helm/$deploymentType directory there is only a single values file. Didn't we talk about having multiple values files, one for each combination of java version + kind? It was a while ago, so maybe we changed our minds, but thats how I thought it was supposed to be.

Those files you see are generated by the quarkus-helm extension.
IIRC, those files are "defaults" for helm values.
I still couldn´t find a way to avoid generating those value files, but, tbh, I didn´t bother much, as we don´t use those files nor those values.

The values you refer to are in the /super-heroes/scripts folder, named values-java17.yml or values-native.yml.

3. After running in each project I notice there is a $project/deploy/helm/$deploymentType/charts directory which is empty. Is this expected?

I think that yes, it is expected.
IIUC, in this folder we place other charts that are hard dependencies for the current chart (https://v2.helm.sh/docs/glossary/#chart-dependency-subcharts).
I need to look further into this, but I suspect we can use this approach for creating the charts for both the rest-fight service and/or the uber-chart.

4. After running in each project I notice there is a $project/deploy/helm/$deploymentType/README.md file which mentions how to install the chart
   
   Those instructions are not correct. The correct command should be helm install -f values.yaml chart-name .

This file is also generated by quarkus-helm.
I guess that, if the instructions are not right, we can just disable the generation of the file with https://quarkiverse.github.io/quarkiverse-docs/quarkus-helm/dev/index.html#quarkus-helm_quarkus.helm.create-readme-file

5. I tried to install each individual chart in this order:

   1. rest-villains
   2. rest-heroes
   3. event-statistics
   4. rest-fights

   When installing the rest-fights chart, I got this error:

   ╰─ helm install -f values.yaml rest-fights .     
   Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: RoleBinding "default_view" in namespace "eric-deandrea-dev" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "rest-fights"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "eric-deandrea-dev"

   I think this is because both event-statistics and rest-fights both have definitions for the kafka/apicurio stuff since they both use it. There probably needs to be adjustments in the charts for both those apps to only deploy those resources if they don't already exist. When we kubctl apply -f its essentially a no-op because kubectl apply -f will ignore resources that are identical. Helm does not.

🤔 I would need more guidance here.
I can´t perform the helm install locally, as I have no k8s instance available (yet), so I can´t replicate. But I just can find a single RoleBinding in the generated files, belonging to the rest-fights app.
Maybe is it due to the latest changes I did? Can you give it another try?

@someth2say any chance you can maybe push the helm charts to another git folder somewhere so I can try installing it on Openshift? and see if i get this error (though it seems you already may have got the root cause):

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: RoleBinding "default_view" in namespace "eric-deandrea-dev" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "rest-fights"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "eric-deandrea-dev"

@someth2say any chance you can maybe push the helm charts to another git folder somewhere so I can try installing it on Openshift? and see if i get this error:

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: RoleBinding "default_view" in namespace "eric-deandrea-dev" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "rest-fights"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "eric-deandrea-dev"

Just do gh pr checkout 291 to checkout this pr, then cd to the root directory and run scripts/generate-k8s-resources.sh. Should take about 3 minutes and it will generate everything.

I can´t perform the helm install locally, as I have no k8s instance available (yet), so I can´t replicate. But I just can find a single RoleBinding in the generated files, belonging to the rest-fights app.
Maybe is it due to the latest changes I did? Can you give it another try?

I use Red Hat Developer Sandbox (https://red.ht/dev-sandbox) :) Thats where I tried the helm install.

Its more than just the RoleBinding. Its the Deployment/ConfigMap/Service/etc for the other resources (Kafka, Apicurio, etc). They exist in both rest-fights and event-statistics.

If you look at any of the k8s descriptors in /deploy/k8s you'll notice there are duplicated objects (apicurio Deployment, fights-kafka Deployment, etc).

When you kubectl apply -f these descriptors the duplicate entries are just no-ops. It is smart enough not to do anything if the object is identical to what is already deployed. Seems helm doesn't understand that, so we'll need to figure something out.

so testing on openshift 4.13, I got a similar error with the resources relating to ownership but not exactly what you saw :

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: Service "fights-kafka" in namespace "rest-fights" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-name" must equal "monitoring": current value is "rest-fights"

i did it in the order of rest-villains -> rest-heroes-> rest-fights-> and then event-statistics. For helm only 1 chart can manage the resource, so it might not be an issue when you do the uber chart if its handpicked but I'm not sure how to handle it in this case when it's automatically generated

i did it in the order of rest-villains -> rest-heroes-> rest-fights-> and then event-statistics. For helm only 1 chart can manage the resource, so it might not be an issue when you do the uber chart if its handpicked but I'm not sure how to handle it in this case when it's automatically generated

Yeah the order you do it will affect the error you see, but generally the problem is because both rest-fights and event-statistics both define overlapping resources (kafka/apicurio/etc).

So yes you are right @ingmarfjolla that the uber chart will be a problem, but so will deploying each project individually (as you & I have tried to do).

We need to figure out a way around this. Is there an option in helm itself to say "hey skip this resource if it already exists"? I'm sure something like this is not a new problem, as there are plenty of things that get deployed that are shared amongst applications. Any kind of messaging platform (kafka/etc) would be a perfect example.

based on this issue: helm/helm#4824

there are a couple of workarounds. It looks like we could do something with the helm lookup function like this:

{{- if not (lookup "v1" "Service" "" }}
resources template
{{- end}}

but I haven't used it and i'm not sure if's the most elegant solution since I'm not sure how many duplicated resources there are and idk if you want to wrap every resource in a function like that

you can do conditionals also, which might be an easier solution but would also need some hand picking / manual intervention. So something like

values.yaml:

app:
     rest-fights-managing-kafka:true

service.yaml (for the event statistics):

{{if not rest-fights-managing-kafka}}
resource
{{- end}}

sorry if I'm being confusing/not explaining well enough

there are a couple of workarounds. It looks like we could do something with the helm lookup function like this:

I am thinking a bit about this, and I don´t think we could (easily) use helm functions.
The reason is that "we" (developers) do not generate the helm charts, but quarkus-helm does.
So, in order to apply this approach we should either modify the charts after they are generated, or modify quarkus-helm to include the lookup.
None of them look compelling to me.

If you look at any of the k8s descriptors in /deploy/k8s you'll notice there are duplicated objects (apicurio Deployment, fights-kafka Deployment, etc).

Talking from the perspective of a newby: Maybe we should step back, and ask ourselves "We do we have duplicated resources?"
This breaks the DRY principle.
Maybe what I'm gonna say makes no sense, but, if we have our services (rest-fights and event-statistics) relying on another service (apicurio and kafka), then we don´t have those services as independent projects?
This way, we can create the resources for them independently, and install them before any other dependant services.

Talking from the perspective of a newby: Maybe we should step back, and ask ourselves "We do we have duplicated resources?"
This breaks the DRY principle.
Maybe what I'm gonna say makes no sense, but, if we have our services (rest-fights and event-statistics) relying on another service (apicurio and kafka), then we don´t have those services as independent projects?
This way, we can create the resources for them independently, and install them before any other dependant services.

I think the reasoning goes back to one of the original designs/"guiding principles" of the entire system in the first place - make it stupid simple for anyone to deploy it without requiring manual changes/editing files/etc. Just a single command and boom, things are deployed, either individual services or the entire system.

You want to deploy the fights service and all its dependencies? Single kubectl apply -f and its there. Want to deploy another service? Same thing. Single kubectl apply -f and its there with all of its dependencies.